Could this section be its own independent component so that other blocks could re-use it?

It would be super useful when building e.g. alternative gallery blocks or slider blocks.

The component could even include the drop-zone component and support props supported by media-placeholder component. It would basically be generic "add additional media to this block" -wrapper.

I would love for us to revisit, holistically, the appender overall, to give more options and a better overall design going into phase 2. However that is probably best done separately, and not as part of this bugfix. 👍 👍for this as a good iteration.

Hi,
I'm sorry for the delay in answering I missed the notifications I think I probably unsubscribed to them by mistake.

Could this section be its own independent component so that other blocks could re-use it?

It would be super useful when building e.g. alternative gallery blocks or slider blocks.

The component could even include the drop-zone component and support props supported by media-placeholder component. It would basically be generic "add additional media to this block" -wrapper.

I agree, my plan is, for now, merge the enhancements in the gallery get them a little bit tested and then after everything is polished extract and expose a generic component.

Hi @talldan, thank you for your review, I think I addressed all your comments.

Hey @jorgefilipecosta, thanks for reviving this PR. This works well in my testing!

A couple small design notes:

1. The button styling is a little odd — the text is blue, and it almost looks disabled to me? I'd expect the button to share the color/border/shadow of our other secondary buttons:

2. There's a little extra margin below the dotted outline. It looks like it's inheriting the 16px bottom margin for gallery items, but that can be zeroed out so there's even spacing all around it.

Thank you for the review @kjellr I will look into fixing these issues.

My plan was to after merging this PR extract a component that allows rendering the gallery appender. I noticed that the gallery appender being implemented here is just equivalent to MediaPlaceholder without label and icon, with the media library button opening the media library in a different state, and with some custom styles.
It may be possible refactor this changes to use MediaPlaceholder in the gallery appender by adding two additional optional props one that enables a custom style and one that allows opening the media library in a specific state (the add to gallery mode). I will check if this is, in fact, a possibility and update the PR accordingly.

Would it make sense if MediaPlaceholder could take a child component?

Or perhaps it should be called MediaContainer instead:

<MediaContainer>
  <Gallery />
</MediaContainer>

Where <Gallery> could be also <Slideshow> or <TiledGallery> for Jetpack or even something like <VideoPlaylist>. Child component would only care about files array and how to visualize it — the wrapper would take care of handling the files, dropzone, showing a placeholder when there are no files etc. When there are files, wrapper would show the placeholder under the gallery component.

Something like (very much simplified):

function MediaContainer(WrappedComponent) {
  return class extends Component {
    // handle state, file uploads, notices etc

    render() {
      if ( ! this.state.files.length ) {
        return <MediaPlaceholder />
      }

      return (
        <Fragment>
          { this.props.noticeUI }
          <DropZone />
          <WrappedComponent files={ this.state.files } { ...this.props } />
          <MediaPlaceholder title={ false } />
        </Fragment>
      );
    }
  };
}

export default withNotices( MediaContainer );

The code was updated, now instead of custom add item logic MediaPlaceholder handles everything (it got new additions). The gallery code and styles got simplified with this change.
cc: @sirreal, @simison

@kjellr The design was changed and I think your points were addressed:

Hi @simison,
Right now with this change the MediaPlaceholder already fits most use cases and some functions like the one that handles the addition of new files are not required anymore.
In the idea, you are proposing MediaContainer is responsible for rendering the MediaPlaceholder, the MediaPlaceholder contains many block specific parts (icon, labels, mapping of file/media objects to attributes, etc... This props would also need to be passed to MediaContainer so I'm not sure if the gain is significant.
I think we can try to enhance MediaPlaceholder and then we may think of ways to generalize more.

Hi @simison,
I remember that gallery had a design for the add button equivalent to the one you shared but it was meanwhile changed and the add button is already wide on master:

I rebased the PR did a new set of test. I reverted the fix to hoover spacing problem -- the fix caused a bug when the gallery is in the placeholder state, and I noticed this problem is already present on master. I would prefer if we address the problem separately.

@jasmussen @kjellr any thoughts regarding the current state of this PR? Any task remaining in your opinion?

@jasmussen @kjellr any thoughts regarding the current state of this PR? Any task remaining in your opinion?

This is looking good. Just one remaining item in my opinion: We should set it so that a click anywhere on the drop zone triggers the upload modal.

I do feel like we should consider changing the dropzone area as described above #12367 (comment), but that'd come with some tradeoffs, so I think it's fine to punt it for now and revisit later. 👍

Hi @kjellr, this PR was updated and all the appender area is clickable.